Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JBTM-3005 Update to add a quickstart with the commmon-dbcp2 and tomcat #221

Merged
merged 1 commit into from Apr 11, 2018

Conversation

zhfeng
Copy link
Contributor

@zhfeng zhfeng commented Apr 4, 2018

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@tomjenkinson
Copy link
Contributor

@zhfeng is this ready for review?

@zhfeng
Copy link
Contributor Author

zhfeng commented Apr 9, 2018

It needs to add a recovery test.

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@tomjenkinson tomjenkinson self-requested a review April 10, 2018 08:32
for i in {1..10}
do
curl -f --data "test$i" http://localhost:8080/${QUICKSTART_NAME}
done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add in something that calls the crash endpoint and then the recover one and verifies the json has the new string dao please?

Copy link
Contributor Author

@zhfeng zhfeng Apr 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I will do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

* @throws SQLException
*/
public void save(String string) throws SQLException {
// Connection must be closed by transaction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a restriction of DBCP2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so. These codes are copied from the transactional driver quickstart, So it could remove these comments and invoke the close() method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Please can you try that?

@ochaloup
Copy link
Contributor

@zhfeng : I'm interested in one thing - should not be the class TransactionalDataSourceFactory (https://github.com/zhfeng/quickstart/blob/b203e83db6aab9605322b40ce6cd0997fd7cda6b/dbcp2-and-tomcat/src/main/java/io/narayana/TransactionalDataSourceFactory.java, or some similar) being part of the Narayana release (maybe tomcat-jta?). I mean it seems to me that whoever will want to integrate tomcat-dbcp2-narayana will need to copy&paste this class to his project, isn't that so?

@Karm
Copy link

Karm commented Apr 10, 2018

@ochaloup I just put TransactionalDataSourceFactory it in tomcat-jta/src/main/java/org/jboss/narayana/tomcat/jta/ and I think there is a consensus it could stay there. With the whole Tomcat JTA module possibly moving to our upstream web-servers Github org as outlined on JBTM-3005

@zhfeng
Copy link
Contributor Author

zhfeng commented Apr 11, 2018

@ochaloup yeah, we should have this factory class in the tomcat-jta integration codebase.

@tomjenkinson
Copy link
Contributor

I agree with you, please can you raise the JIRA to do that?

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@ochaloup
Copy link
Contributor

@zhfeng @Karm I see, thanks for the details

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@tomjenkinson tomjenkinson merged commit cab0e97 into jbosstm:master Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants